Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Cloud Task Queue resource #4529

Conversation

mccall114
Copy link
Contributor

New resource google_task_queue as part of issue #2367.

First time contributor here. I read the guidelines about splitting out new packages in a separate PR, and I'm happy to do that if this one is ok.

@mccall114
Copy link
Contributor Author

Linter failed in travis. Running locally with go 1.11.13 confirms that it's failing on code I didn't touch.

Copy link

@jbornemann jbornemann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm 👍

}

func resourceTaskQueueImportState(d *schema.ResourceData, meta interface{}) ([]*schema.ResourceData, error) {
parts := strings.Split(d.Id(), "/")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider doing what resource_kms_key_ring.go does for its import, using parseImportId() and replaceVars().

@rileykarson rileykarson requested a review from tysen October 8, 2019 22:21
},
},
},
"retry": {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer if this matched the API: retry_config

Comment on lines 30 to 35
Check: resource.ComposeTestCheckFunc(
testAccCheckTaskQueueExists(
"google_task_queue.fizzbuzz", location, queueName, &queue),
testAccCheckTaskQueueEquals(
"google_task_queue.fizzbuzz", &queue),
),
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need these checks (nor the funcs, and I think that also means we can avoid vendoring this new context dependency) - the ImportStateVerify is sufficient.

@mccall114
Copy link
Contributor Author

@tysen, I've removed those tests and renamed retry to retry_config. I left max_burst_size and host so that we could still see them in the output. LMK what you think.


expectedName := fmt.Sprintf("projects/%s/locations/%s/queues/%s", config.Project, attributes["location"], attributes["name"])

_, err := config.clientCloudTasks.Projects.Locations.Queues.Get(expectedName).Context(context.Background()).Do()
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why include this call to Context? Does this not work without it like the other services?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay, I was on vacation. I've removed the call to Context.

@jba
Copy link

jba commented Oct 27, 2019

I'd love to start using this. What is holding it up? Can I help?

@tysen
Copy link

tysen commented Oct 28, 2019

Still waiting for a response to the question in my review.

@pokojskis
Copy link

I would really love to use it

@tysen
Copy link

tysen commented Oct 30, 2019

PR looks good now. If you could submit separate PRs (one for TPG and one for TPGB) to vendor the new dependency (and then rebase this PR to remove the vendoring here), afterwards I can upstream this PR into MM and get it merged.

@mccall114 mccall114 force-pushed the issue-2367-cloud-task-queue-resources branch from d7f79b2 to fbfcea0 Compare October 30, 2019 20:44
@mccall114 mccall114 force-pushed the issue-2367-cloud-task-queue-resources branch from fbfcea0 to 070e0bf Compare October 30, 2019 20:53
@ghost ghost added size/xl and removed size/xxl labels Oct 30, 2019
@mccall114
Copy link
Contributor Author

@tysen, I've rebased this branch and opened #4781
and hashicorp/terraform-provider-google-beta#1315.

@tysen
Copy link

tysen commented Nov 5, 2019

@mccall114 Looks like you need to sign the CLA GoogleCloudPlatform/magic-modules#2587

@mccall114
Copy link
Contributor Author

@tysen I just signed it

@tysen
Copy link

tysen commented Nov 5, 2019

@mccall114 Thanks, will you post @googlebot I fixed it. on that PR?

@seriousManual
Copy link

any updates on this?

@tysen
Copy link

tysen commented Nov 13, 2019

I had some issues finalizing this implementation, and it'll be easier to maintain and extend if it's generated by Magic Modules, so I submitted GoogleCloudPlatform/magic-modules#2662 which is under review.

@tysen
Copy link

tysen commented Nov 18, 2019

Should be in the next release: GoogleCloudPlatform/magic-modules#2662

@tysen tysen closed this Nov 18, 2019
@mccall114
Copy link
Contributor Author

Thanks @tysen!

@ghost
Copy link

ghost commented Dec 19, 2019

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 hashibot-feedback@hashicorp.com. Thanks!

@ghost ghost locked and limited conversation to collaborators Dec 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants